stream: adding new 'data' handler doesn't resume stream after removing 'readable' handler #24474

tvald · 2018-11-19T05:26:52Z

Version: 11.2.0
Platform: Windows 10 x64
Subsystem: Streams

Description
After a readable handler has been added and removed from a stream, the stream no longer begins / resumes flowing when a data handler is added to the stream. This is a breaking change from 10.x and appears inconsistent with the (convoluted) intended behavior of streams. (More on this below.)

Related issues: #22209, #24366, #24281

Example

const stream = fs.createReadStream('foo.txt'); // some file that exists

const onData = () => { console.log('DATA'); };
const onReadable = () => {
  console.log('READABLE');
  stream.off('readable', onReadable);
  stream.on('data', onData);
  //stream.resume(); // <-- this causes data to be emitted
};

stream.on('readable', onReadable);

Expected Output:
The readable handler is called, and then the data handler is called.

Actual behavior:
The readable handler is called, but not the data handler.

Further Discussion
It is desirable for a stream to resume flowing when no readable handler is attached and a data handler is added. For example, one might generate a stream and check that it successfully opens (via the readable handler) before returning the stream to be consumed elsewhere (via the data handler). This worked at least up through 10.x.

With a certain reading, the current gap in behavior could be held as consistent with the "Three States" / "under the hood" explanation of stream modes, although that in itself may contradict the "Two Reading Modes" abstraction.

From streams documentation:
Two Reading Modes

All Readable streams begin in paused mode but can be switched to flowing mode in one of the following ways: (1) Adding a data event handler...

Adding a readable event handler automatically makes the stream to stop flowing, and the data to be consumed via readable.read(). If the readable event handler is removed, then the stream will start flowing again if there is a data event handler.

Three States

Calling readable.pause(), readable.unpipe(), or receiving backpressure will cause the readable.readableFlowing to be set as false, temporarily halting the flowing of events but not halting the generation of data. While in this state, attaching a listener for the 'data' event will not switch readable.readableFlowing to true.

Without having looked at code, I interpret the "Three States" description to indicate that there is no way to return from the readableFlowing == false state to the readableFlowing == null state. Hence, adding a readable handler destroys the auto-start behavior of the data handler.

This is sufficiently counter-intuitive that a slew of issues have been filed in the past few months, such as #24366 and #24281. In fact, a patch was even accepted in #22209 that ensures the auto-start behavior of a data handler IFF it was added before the readable handler.

The text was updated successfully, but these errors were encountered:

Trott · 2018-11-25T18:46:41Z

@nodejs/streams

lpinca · 2019-07-16T20:01:14Z

For the example in the description you can use stream.removeListener() instead of stream.off() and it will work as expected. However if the 'data' listener if added on next tick (or later) the issue persists. Here is a test case:

'use strict';

const { Readable } = require('stream');

const readable = new Readable({
  read() {}
});

function read() {}

readable.setEncoding('utf8');
readable.on('readable', read);
readable.removeListener('readable', read);

process.nextTick(function() {
  readable.on('data', function(chunk) {
    console.log(chunk);
  });

  readable.push('hello');
});

The expected behavior is to have 'hello' printed on the console but nothing is actually printed because flowing is set to false

node/lib/_stream_readable.js

Line 830 in 7032e59

state.flowing = false;

when the 'readable' listener is added, and updateReadableListening()

node/lib/_stream_readable.js

Lines 877 to 890 in 7032e59

    
           function updateReadableListening(self) { 
        
             const state = self._readableState; 
        
             state.readableListening = self.listenerCount('readable') > 0; 
        
             if (state.resumeScheduled && !state.paused) { 
        
               // Flowing needs to be set to true now, otherwise 
        
               // the upcoming resume will not flow. 
        
               state.flowing = true; 
        
               // Crude way to check if we should resume 
        
             } else if (self.listenerCount('data') > 0) { 
        
               self.resume(); 
        
             } 
        
           }

does not find any registered 'data' listener.

I'm not sure if this is actually intended behavior but it is the reason why https://github.com/TooTallNate/node-https-proxy-agent no longer work in Node.js >= 10.0.0 and that lib has ~6 millions weekly downloads (I think we should add it to CITGM).

cc: @nodejs/streams @mcollina

Resume the socket after the `'socket'` event is emitted on the `ClientRequest` object. Refs: nodejs/node#24474 (comment) Fixes: TooTallNate#58

discussion: https://github.com/nodejs/node/issues/24474\#issuecomment-511963799

@jennifer-shehane

* fix specs * use debugger protocol for cookie handling in electron * use latest gulp * use rimraf instead of gulp-clean * use electron 3.1.8 and node 10.2.1 * use gulp 4 in packages/static * fix sendCommandAsync, log Schema.getDomains on CDP connect * autofill e2e test name [skip ci] * electron@5.0.7, see what new failures exist * --no-sandbox for launching Electron * update cookies logic for electron * node 12 * update snapshot for new node * update error message for new node * stub sendCommandAsync * only connect to socket if path has been replaced, fixes #4776 * update node-sass to support node 12 * skip wacky socket tests for now * snapshot * fix run_plugins_spec snapshot, don't include stack trace * use --no-sandbox on linux to run as root * allow sendCommandAsync to resolve * use euid for root check * log domains even if undefined * don't worry about ending 1xx responses immediately anymore * use --max-http-header-size, change max size from 8kb to 1mb, fix #76 * do not send 502 on failed websocket, just send back ECONNRESET * update websocket spec port to not collide with other test * update outdated expect * Revert "only connect to socket if path has been replaced, fixes #4776" This reverts commit f179eda. * update gulp in root * update https-proxy unit tests * update network spec to properly close server * update reporter spec * update https-proxy-agent to fix node 10.10.0 change discussion: https://github.com/nodejs/node/issues/24474\#issuecomment-511963799 * only pass --max-http-header-size on node >=12 * use own server-destroy implementation that supports secureConnect events * oops * update socket_spec * electron 6.0.0 * console.table introduced in node 10 * change browserify entry to init.js * handle edge case when no response body * console.table added in node 10 * do not exit app when all BrowserWindows are closed * update e2e snapshots * value may not be null * update plugins spec * correct cookie expiry, use browser.getversion for CDP version check * fix snapshotting for require stacks * reorder cookies in spec * warn when depreated electron callback apis are used * only report 1 plugin error per process * cleanup * node 12.4.0, cypress/browsers:node12.4.0-chrome76 docker image * update shell.openExternal to promisified * update dialog.showOpenDialog to promisified * update webContents.session.setProxy to promisified * updating native dependencies since we don't need ancient node ABI support anymore * WIP: switch cookies to simpler, jar-less approach * WIP: switch cookies to simpler, jar-less approach * making tests pass * improve cookie filtering logic * Remove unneeded Promise.try * filter what makes it to the extension * properly re-set superdomain cookies on cross-origin cy.visit * allow comma-separated list of e2e tests * sort cookies in order of expiration date, ascending * updating tests, cleanup * update tests * version electron as a devDependency, electron@6.0.1 * cleanup, remove old automation * cleanup, remove old automation * bump chokidar to fix win10 + node12 issue was seeing this on windows: nuxt/nuxt#6035 fixed with version bump * enable now-supported quit role, re-enable old tests * don't need that arg there * remove last deprecated callback electron invocations * Delete cypress.json * responding to PR feedback * cleanup * invoke * use 'quit' role * Use new appMenu role for Cypress menu on mac * electron@6.0.2 * electron@6.0.3 * remove domain: cookie.domain and see what happens * remove setErrorHandler * Revert "remove domain: cookie.domain and see what happens" This reverts commit 49e9168. * add unit tests for cookies * ci * fix project-content css * electron@6.0.4 * fix specs_list test * electron@6.0.7 * some cleanup * electron@6.0.9 * Update 8_reporters_spec.coffee.js * electron@5.0.10 - Chromium 73, Node 12 * cli: fix the STDIN pipe on Windows (#5045) * cli: pipe stdin * uggh, here is the actual change * update cli unit tests * add unit test * more permissive check for json to include application/vnd.api+j… (#5166) * more permissive check for json to include * add json test for content-type application/vnd.api+json * cruder solution passes e2e tests locally, so let's go with that * Remove 'charset' from content-type before checking if JSON * fix eslint for fixture specs (#5176) * update eslint to lint files within 'fixtures' in support files - ignore some edge cases like jquery, jsx and obvious js files we wrote with broken code * Fixes from eslint to 'fixtures' files * Catch env variable with reserved name CYPRESS_ENV 1621 (#1626) * server: check CYPRESS_ENV variable when merging configs * catch invalid CYPRESS_ENV value in CLI, close #1621 * linting * sanitize platform in test snapshot * linting * update error message text * add missing comma * fix finally merge in JS code * pass CLI linter * fix log reference, should be debug * use correct sinon reference * update message, show first part in red * update error message text * Addresses #2953 (#5174) * Addresses #2953 * Added proper test for new error message * Didn't realize it ran this test as well, whoops * Implementing changes as suggested by @jennifer-shehane * Fixing tests and error output. Moved the checks to the start of the get command to ensure we always catch improper options * Removing issue test since the querying spec covers it * Using coffescript isArray check * depromisify things that were promisified b/t electron 5 <=> 6 Revert "update shell.openExternal to promisified" This reverts commit 8b6460d. Revert "update dialog.showOpenDialog to promisified" This reverts commit 5f178b0. Revert "update webContents.session.setProxy to promisified" This reverts commit 727df3a. * node12.4.0-chrome76 => node12.0.0-chrome75 * fix tests for electron downgrade * node12.0.0-chrome75 => node12.0.0-chrome73 Co-authored-by: Zach Bloomquist <github@chary.us> Co-authored-by: Brian Mann <brian.mann86@gmail.com>

Resume the socket after the `'socket'` event is emitted on the `ClientRequest` object. Refs: nodejs/node#24474 (comment) Fixes: #58

Resume the socket after the `'socket'` event is emitted on the `ClientRequest` object. Fixes: #58 Refs: nodejs/node#24474 (comment)

@jennifer-shehane

* fix specs * use debugger protocol for cookie handling in electron * use latest gulp * use rimraf instead of gulp-clean * use electron 3.1.8 and node 10.2.1 * use gulp 4 in packages/static * fix sendCommandAsync, log Schema.getDomains on CDP connect * autofill e2e test name [skip ci] * electron@5.0.7, see what new failures exist * --no-sandbox for launching Electron * update cookies logic for electron * node 12 * update snapshot for new node * update error message for new node * stub sendCommandAsync * only connect to socket if path has been replaced, fixes cypress-io#4776 * update node-sass to support node 12 * skip wacky socket tests for now * snapshot * fix run_plugins_spec snapshot, don't include stack trace * use --no-sandbox on linux to run as root * allow sendCommandAsync to resolve * use euid for root check * log domains even if undefined * don't worry about ending 1xx responses immediately anymore * use --max-http-header-size, change max size from 8kb to 1mb, fix cypress-io#76 * do not send 502 on failed websocket, just send back ECONNRESET * update websocket spec port to not collide with other test * update outdated expect * Revert "only connect to socket if path has been replaced, fixes cypress-io#4776" This reverts commit f179eda. * update gulp in root * update https-proxy unit tests * update network spec to properly close server * update reporter spec * update https-proxy-agent to fix node 10.10.0 change discussion: https://github.com/nodejs/node/issues/24474\#issuecomment-511963799 * only pass --max-http-header-size on node >=12 * use own server-destroy implementation that supports secureConnect events * oops * update socket_spec * electron 6.0.0 * console.table introduced in node 10 * change browserify entry to init.js * handle edge case when no response body * console.table added in node 10 * do not exit app when all BrowserWindows are closed * update e2e snapshots * value may not be null * update plugins spec * correct cookie expiry, use browser.getversion for CDP version check * fix snapshotting for require stacks * reorder cookies in spec * warn when depreated electron callback apis are used * only report 1 plugin error per process * cleanup * node 12.4.0, cypress/browsers:node12.4.0-chrome76 docker image * update shell.openExternal to promisified * update dialog.showOpenDialog to promisified * update webContents.session.setProxy to promisified * updating native dependencies since we don't need ancient node ABI support anymore * WIP: switch cookies to simpler, jar-less approach * WIP: switch cookies to simpler, jar-less approach * making tests pass * improve cookie filtering logic * Remove unneeded Promise.try * filter what makes it to the extension * properly re-set superdomain cookies on cross-origin cy.visit * allow comma-separated list of e2e tests * sort cookies in order of expiration date, ascending * updating tests, cleanup * update tests * version electron as a devDependency, electron@6.0.1 * cleanup, remove old automation * cleanup, remove old automation * bump chokidar to fix win10 + node12 issue was seeing this on windows: nuxt/nuxt#6035 fixed with version bump * enable now-supported quit role, re-enable old tests * don't need that arg there * remove last deprecated callback electron invocations * Delete cypress.json * responding to PR feedback * cleanup * invoke * use 'quit' role * Use new appMenu role for Cypress menu on mac * electron@6.0.2 * electron@6.0.3 * remove domain: cookie.domain and see what happens * remove setErrorHandler * Revert "remove domain: cookie.domain and see what happens" This reverts commit 49e9168. * add unit tests for cookies * ci * fix project-content css * electron@6.0.4 * fix specs_list test * electron@6.0.7 * some cleanup * electron@6.0.9 * Update 8_reporters_spec.coffee.js * electron@5.0.10 - Chromium 73, Node 12 * cli: fix the STDIN pipe on Windows (cypress-io#5045) * cli: pipe stdin * uggh, here is the actual change * update cli unit tests * add unit test * more permissive check for json to include application/vnd.api+j… (cypress-io#5166) * more permissive check for json to include * add json test for content-type application/vnd.api+json * cruder solution passes e2e tests locally, so let's go with that * Remove 'charset' from content-type before checking if JSON * fix eslint for fixture specs (cypress-io#5176) * update eslint to lint files within 'fixtures' in support files - ignore some edge cases like jquery, jsx and obvious js files we wrote with broken code * Fixes from eslint to 'fixtures' files * Catch env variable with reserved name CYPRESS_ENV 1621 (cypress-io#1626) * server: check CYPRESS_ENV variable when merging configs * catch invalid CYPRESS_ENV value in CLI, close cypress-io#1621 * linting * sanitize platform in test snapshot * linting * update error message text * add missing comma * fix finally merge in JS code * pass CLI linter * fix log reference, should be debug * use correct sinon reference * update message, show first part in red * update error message text * Addresses cypress-io#2953 (cypress-io#5174) * Addresses cypress-io#2953 * Added proper test for new error message * Didn't realize it ran this test as well, whoops * Implementing changes as suggested by @jennifer-shehane * Fixing tests and error output. Moved the checks to the start of the get command to ensure we always catch improper options * Removing issue test since the querying spec covers it * Using coffescript isArray check * depromisify things that were promisified b/t electron 5 <=> 6 Revert "update shell.openExternal to promisified" This reverts commit 8b6460d. Revert "update dialog.showOpenDialog to promisified" This reverts commit 5f178b0. Revert "update webContents.session.setProxy to promisified" This reverts commit 727df3a. * node12.4.0-chrome76 => node12.0.0-chrome75 * fix tests for electron downgrade * node12.0.0-chrome75 => node12.0.0-chrome73 Co-authored-by: Zach Bloomquist <github@chary.us> Co-authored-by: Brian Mann <brian.mann86@gmail.com>

This package has ~6 million monthly downloads and we broke it in Node.js 10.0.0 Refs: nodejs/node#24474 (comment)

ronag · 2019-12-20T14:42:54Z

For the example in the description you can use stream.removeListener() instead of stream.off() and it will work as expected.

I believe this part of the problem was resolved in 1665a93

If we don't have any 'readable' or 'data' listeners and we are not about to resume. Then reset flowing state to initial null state. Fixes: nodejs#24474

If we don't have any 'readable' or 'data' listeners and we are not about to resume. Then reset flowing state to initial null state. PR-URL: #31036 Fixes: #24474 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

This package has ~6 million monthly downloads and we broke it in Node.js 10.0.0 Refs: nodejs/node#24474 (comment)

Resume the socket after the `'socket'` event is emitted on the `ClientRequest` object. Refs: nodejs/node#24474 (comment) Fixes: TooTallNate/proxy-agents#58

Resume the socket after the `'socket'` event is emitted on the `ClientRequest` object. Fixes: #58 Refs: nodejs/node#24474 (comment)

Trott added stream Issues and PRs related to the stream subsystem. windows Issues and PRs related to the Windows platform. labels Nov 25, 2018

mscdex mentioned this issue Jan 22, 2019

stream: readable.resume() not allowing 'end' to be emitted after readable.unshift() #25642

Closed

lpinca removed the windows Issues and PRs related to the Windows platform. label Jul 17, 2019

lpinca added a commit to lpinca/node-https-proxy-agent that referenced this issue Jul 26, 2019

Fix compatibility with Node.js >= 10.0.0

ec7c3ec

Resume the socket after the `'socket'` event is emitted on the `ClientRequest` object. Refs: nodejs/node#24474 (comment) Fixes: TooTallNate#58

lpinca mentioned this issue Jul 26, 2019

Fix compatibility with Node.js >= 10.0.0 TooTallNate/proxy-agents#73

Closed

flotwig referenced this issue in cypress-io/cypress Jul 30, 2019

update https-proxy-agent to fix node 10.10.0 change

9973d18

discussion: https://github.com/nodejs/node/issues/24474\#issuecomment-511963799

flotwig mentioned this issue Jul 30, 2019

Electron v5.0.10 cypress-io/cypress#4720

Merged

TooTallNate pushed a commit to TooTallNate/proxy-agents that referenced this issue Oct 4, 2019

Fix compatibility with Node.js >= 10.0.0

2629ba6

Resume the socket after the `'socket'` event is emitted on the `ClientRequest` object. Refs: nodejs/node#24474 (comment) Fixes: #58

TooTallNate added a commit to TooTallNate/proxy-agents that referenced this issue Oct 4, 2019

Fix compatibility with Node.js >= 10.0.0 (#73)

c7d8161

Resume the socket after the `'socket'` event is emitted on the `ClientRequest` object. Fixes: #58 Refs: nodejs/node#24474 (comment)

lpinca added a commit to lpinca/citgm that referenced this issue Oct 9, 2019

lookup: add https-proxy-agent

8ed095d

This package has ~6 million monthly downloads and we broke it in Node.js 10.0.0 Refs: nodejs/node#24474 (comment)

lpinca mentioned this issue Oct 9, 2019

lookup: add https-proxy-agent nodejs/citgm#752

Merged

2 tasks

semantic-commit bot mentioned this issue Nov 21, 2019

Issue 144 TomCaserta/cypress#1

Closed

ronag mentioned this issue Dec 20, 2019

stream: reset flowing state if no 'readable' or 'data' listeners. #31036

Closed

4 tasks

BridgeAR closed this as completed in 3d47c85 Dec 25, 2019

lpinca added a commit to lpinca/citgm that referenced this issue May 16, 2020

lookup: add https-proxy-agent

89a71f3

This package has ~6 million monthly downloads and we broke it in Node.js 10.0.0 Refs: nodejs/node#24474 (comment)

lpinca added a commit to lpinca/citgm that referenced this issue Apr 21, 2022

lookup: add https-proxy-agent

d3acc07

This package has ~6 million monthly downloads and we broke it in Node.js 10.0.0 Refs: nodejs/node#24474 (comment)

targos pushed a commit to nodejs/citgm that referenced this issue Apr 22, 2022

lookup: add https-proxy-agent (#752)

7089166

This package has ~6 million monthly downloads and we broke it in Node.js 10.0.0 Refs: nodejs/node#24474 (comment)

adrianchu0120 added a commit to adrianchu0120/proxy-agents that referenced this issue Aug 25, 2024

Fix compatibility with Node.js >= 10.0.0 (#73)

56ee59e

Resume the socket after the `'socket'` event is emitted on the `ClientRequest` object. Fixes: #58 Refs: nodejs/node#24474 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: adding new 'data' handler doesn't resume stream after removing 'readable' handler #24474

stream: adding new 'data' handler doesn't resume stream after removing 'readable' handler #24474

tvald commented Nov 19, 2018

Trott commented Nov 25, 2018

lpinca commented Jul 16, 2019

ronag commented Dec 20, 2019

stream: adding new 'data' handler doesn't resume stream after removing 'readable' handler #24474

stream: adding new 'data' handler doesn't resume stream after removing 'readable' handler #24474

Comments

tvald commented Nov 19, 2018

Trott commented Nov 25, 2018

lpinca commented Jul 16, 2019

ronag commented Dec 20, 2019